feat: (PRO-345) Fixed inner instructions#228
Merged
dev-jodee merged 4 commits intorelease/feature-freeze-for-auditfrom Sep 30, 2025
Merged
Conversation
- Inner instructions weren't properly being handled - Added parsing of Parsed / PartiallyDecoded instructions coming back from the RPC - Added tests with inner instructions to make sure the inner instructions are properly used
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to dd3d84a in 3 minutes and 3 seconds. Click for details.
- Reviewed
2480lines of code in10files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. tests/rpc/fee_estimation.rs:90
- Draft comment:
Consider adding dynamic checks/ranges for fee conversion values instead of relying solely on hardcoded expected numbers; this will ensure robustness if pricing or conversion logic changes. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. tests/src/bin/test_runner.rs:112
- Draft comment:
Consider using structured logging instead of println! for production-like test reporting to enhance debuggability and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. tests/src/common/fixtures/kora-test.toml:29
- Draft comment:
Review the allowed programs and tokens to ensure they stay up-to-date with current test requirements. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. tests/src/test_runner/kora.rs:7
- Draft comment:
Consider using an async-aware mutex (e.g., tokio::sync::Mutex) for the global port tracker instead of std::sync::Mutex to avoid potential blocking in async contexts. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. Cargo.lock:7193
- Draft comment:
Typo: The dependency "Inflector" is capitalized. Typically, the crate name is in lowercase ("inflector"). Please verify if this is intentional or if it should be corrected. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment points out something unusual, there are several reasons to delete it: 1) It's about a dependency in Cargo.lock which is auto-generated and not meant for manual editing 2) The comment is speculative, asking to "verify if this is intentional" rather than pointing out a clear issue 3) The capitalization likely comes from the upstream package and isn't something the PR author can control 4) The comment violates the rule about not commenting on dependency changes The capitalization is indeed unusual and could potentially indicate a mistake somewhere. Maybe there's a deeper issue that should be investigated? While unusual, the capitalization of a third-party dependency name in an auto-generated lockfile is not something that requires investigation or comment. The rules explicitly state not to comment on dependency changes or anything in auto-generated files. This comment should be deleted as it violates multiple rules - it's about dependencies, it's in an auto-generated file, and it's speculative rather than pointing out a clear issue.
Workflow ID: wflow_J2tSGoaf72klRgbq
You can customize 
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 803de5c in 1 minute and 1 seconds. Click for details.
- Reviewed
34lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. crates/lib/src/transaction/versioned_transaction.rs:152
- Draft comment:
Ensure the flag change (replace_recent_blockhash: false) is intentional; consider adding a brief comment explaining why false is needed for inner instruction handling. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. tests/src/common/transaction.rs:314
- Draft comment:
Good use of spl_token::state::Account::LEN instead of a magic number; this avoids hard-coded sizes. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_u9Jm3s5p9p6OOWaW
You can customize
amilz
reviewed
Sep 30, 2025
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed aeb91ca in 2 minutes and 45 seconds. Click for details.
- Reviewed
1105lines of code in1files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. crates/lib/src/transaction/instruction_util.rs:265
- Draft comment:
Performance: Consider constructing the account keys hashmap just once rather than rebuilding it in multiple branches (e.g. lines 265, 272, 278). This can avoid redundant work if the same keys are passed in. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. crates/lib/src/transaction/instruction_util.rs:292
- Draft comment:
In the PartiallyDecoded instruction branch, the use of filter_map (lines 294–302) silently drops account keys that fail to parse. Consider handling these errors explicitly to avoid unexpected omissions. - Reason this comment was not posted:
Comment was on unchanged code.
3. crates/lib/src/transaction/instruction_util.rs:609
- Draft comment:
For the SPL token burn (non-checked) case, using unwrap_or(0) when extracting the 'amount' (line 610) may silently default a missing field to 0. It might be more robust to propagate an error instead of defaulting. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a valid concern about error handling. Silently defaulting to 0 could mask issues and lead to unexpected behavior. However, looking at the context, this appears to be reconstruction logic for parsing instructions, not the actual burn execution. The code is trying to be lenient in parsing historical transactions. The unwrap_or(0) pattern is used deliberately here as a fallback. The comment raises a good point about robust error handling. Silent defaults can be dangerous. But am I missing some reason why lenient parsing would be preferable here? Yes - this is instruction reconstruction code that needs to handle historical transactions that may have varying formats. Being strict about parsing could break compatibility. The unwrap_or(0) is an intentional choice for compatibility. The comment should be deleted. While it raises a valid general concern about error handling, the lenient parsing with unwrap_or(0) is appropriate in this specific context of reconstructing historical transaction instructions.
4. crates/lib/src/transaction/instruction_util.rs:256
- Draft comment:
Consider handling errors from bs58 decoding explicitly rather than unwrapping to default (e.g. in uncompile_instructions and in reconstruct_instruction_from_ui). Explicit error logging or propagation could aid debugging if decoding fails. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. crates/lib/src/transaction/instruction_util.rs:197
- Draft comment:
Using a u8 for account index is likely fine given Solana’s constraints, but consider documenting the potential for overflow if the account keys exceed 256 elements. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. crates/lib/src/transaction/instruction_util.rs:248
- Draft comment:
Overall, the new inner-instruction handling and reconstruction logic is comprehensive and well-tested. The helper methods improve code readability. Ensure that the error handling paths (returning None on errors) are aligned with how downstream callers are expected to behave. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. crates/lib/src/transaction/instruction_util.rs:245
- Draft comment:
Typographical error: "force the RPC to not parsed the instructions" should be changed to "force the RPC to not parse the instructions" for grammatical correctness. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment is technically correct about the grammar, it's a very minor issue in a docstring comment. The meaning is still clear even with the grammatical error. The rules state not to make comments that are obvious or unimportant. A small grammatical fix in documentation is not critical enough to warrant a comment. Could incorrect grammar in documentation lead to confusion for non-native English speakers? Should we maintain high standards for documentation quality? While documentation quality is important, this particular error is minor and doesn't impact understanding. The meaning is still clear even with the grammatical error. Delete the comment. The grammatical fix is too minor to warrant a PR comment.
Workflow ID: wflow_cu58a1cEAIACRGX5
You can customize
amilz
approved these changes
Sep 30, 2025
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ce2478a in 2 minutes and 1 seconds. Click for details.
- Reviewed
72lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. crates/lib/src/transaction/instruction_util.rs:262
- Draft comment:
Avoid using unwrap_or_default() when decoding the bs58 data. Silently defaulting may hide decoding errors. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. crates/lib/src/transaction/instruction_util.rs:290
- Draft comment:
Parsing account strings in partially decoded instructions silently drops failures via filter_map. Consider handling parse errors explicitly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. crates/lib/src/transaction/instruction_util.rs:277
- Draft comment:
For unsupported instruction types, a default stub is returned. Consider adding more documentation or even error‐handling so that unexpected instruction types aren’t silently ignored. - Reason this comment was not posted:
Comment was on unchanged code.
4. crates/lib/src/transaction/instruction_util.rs:267
- Draft comment:
The account_keys_hashmap is built separately in multiple branches. If performance becomes critical, consider caching it instead of recomputing. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
5. crates/lib/src/transaction/instruction_util.rs:213
- Draft comment:
The helper function build_default_compiled_instruction returns a stub with empty accounts and data. A brief comment explaining its use (especially for inner instructions) would improve clarity. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
Workflow ID: wflow_dNFsVMsGbY5RVTtQ
You can customize
amilz
approved these changes
Sep 30, 2025
dev-jodee
deleted the
feat/pro-345-added-inner-instructions-parsing-and-testing
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Enhanced inner instruction handling by adding parsing for
ParsedandPartiallyDecodedinstructions, with comprehensive tests and updated dependencies.ParsedandPartiallyDecodedinstructions ininstruction_util.rs.fetch_inner_instructions()inversioned_transaction.rsto handle new instruction types.instruction_util.rsandversioned_transaction.rs.solana-transaction-statustoCargo.tomlandCargo.lock.spl-associated-token-accountversion inCargo.lock.fee_estimation.rsfor transaction fee estimation with various scenarios.test_runner.rsandtransaction.rsto support new test cases.This description was created by
for ce2478a. You can customize this summary. It will automatically update as commits are pushed.
📊 Unit Test Coverage
Unit Test Coverage: 85.4%
View Detailed Coverage Report